-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose the the cradle config output #62
base: master
Are you sure you want to change the base?
Conversation
This restructures the code so that the selected configuration (whether a yaml-based cradle or an implicit one) is exposed in the API. As a result of the rearrangement there is no longer any need to separately expose the implicit, default, and yaml cradle finding functions.
In ghcide we need to know cheaply if two files get the same config. That doesn't seem possible anymore? We previously used the output of findCradle as the key. |
@ndmitchell I restored |
`cradleConfig` now uses `findCradle` to get the configuration path.
@fendor I apologise I think I accidentally somehow managed to delete your other review request. > cradleConfig "exe/Main.hs"
Just (CradleConfig {cradleDependencies = [], cradleType = Cabal {component = Just "lib:hie-bios"}},".") If I don't have a Just (CradleConfig {cradleDependencies = [], cradleType = Cabal {component = Nothing}},".") These are what I expect. Am I missing something? |
I deleted it, because I realized I made a mistake! |
, loadCradle | ||
, loadImplicitCradle | ||
, defaultCradle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their only use was from Main.hs
and I thought it would be better to encapsulate Cradle selection to a single function. Do you want me to revert it?
I don't really understand the API changes in this PR. Do I understand right the goal is to expose the selected |
@mpickering exposing |
I'm not sure what is better but would this API work?
|
@mpickering yes and I think your suggestion is cleaner. I can implement that either as an amendment to this PR or another one from scratch? |
Or perhaps
|
does that mean |
I'm also not sure that The difference between |
Yes exactly. |
Yes, it wouldn't because the |
Let me know if you want me to implement this with the interface you suggested and whether you want it as a separate PR or as an amendment to this one and I'll get on it. |
@madgen It would be good if you could implement this interface. Either here or a separate PR is fine. |
I was looking into this ticket again today and it seems a bit awkward to implement. You really want to distinguish between the case where there's a I think the current API makes expressing this quite easy unless I am missing something. |
@mpickering Yes, I've found some problems with the suggested API as well. Sorry, I just haven't got around to forming coherent thoughts. I'll take some time to do it tomorrow. |
This restructures the code so that the selected configuration (whether a yaml-based or an implicit) is exposed in the API.
As a result of the rearrangement there is no longer any need to separately expose the implicit, default, and yaml cradle finding functions.
Basically, I want to use
hie-bios
to find the environment variables used while building a project for that I need access to the configuration it chooses. This is related to https://github.com/digital-asset/ghcide/issues/124.I'm not entirely comfortable with the expected output, so if someone could test that these changes preserve behaviour, it would be great.